Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using get_default_context() for ssl context to avoid blocking call #791

Merged
merged 1 commit into from
Oct 4, 2024

Conversation

iprak
Copy link
Contributor

@iprak iprak commented Sep 6, 2024

This addresses the I/O blocking warning as recommended in https://developers.home-assistant.io/docs/asyncio_blocking_operations/#load_default_certs
Closes #790

@iprak
Copy link
Contributor Author

iprak commented Sep 9, 2024

@ollo69 Please review

@voradortc
Copy link

You should also remove line 15 (import ssl) as it's no longer required.

@@ -27,6 +26,7 @@
import uuid

import aiohttp
from homeassistant.util.ssl import get_default_context
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not make sense because core_async should not depend on home assistant. If in the future this will be implemented as a separate library, cannot have this dependencies.
There is already a way to directly use the aiohttp session created in ha, probably the best solution is to use this as default option (at this moment is available as advanced option during initial configuration)
To implement this in the library in not blocking way we should implement the ha get_default_context and not import the HA method.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked the implementation of get_default_context in homeassistant.util.ssl and I couldn't see any significant difference with what the current ha-smartthinq-sensors code is doing 🤔
Maybe the @cache decorator helps somehow?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be, you should check what @cache decorator do...

Copy link

@fgimenezm fgimenezm Sep 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked. It basically adds a thin wrapper to cache the responses if there is another call to the same function with the same parameters https://docs.python.org/3/library/functools.html#functools.cache
but you are kind of doing that already with _get_session()

Looks like HA is not complaining about using ssl.create_default_context(), it's complaining about where it's being used. Unless the session can be created somewhere else outside of the event loop and then re-used here it's going be complicated.

Also, about this PR... isn't it changing the SSL ciphers of the default context for all HA? Isn't that risky?

Do we still need the _LG_SSL_CIPHERS ? Maybe they updated the servers and the defaults work now

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no cipher set in get_default_context so that part should be the same.

Yes, the warning can be resolved by creating the context outside the event loop but I don't think that can be done in the life cycle of an integration initialization and without referencing ha.

@@ -146,7 +146,7 @@ def _oauth_info_from_result(result_info: dict) -> dict:

def lg_client_session() -> aiohttp.ClientSession:
"""Create an aiohttp client session to use with LG ThinQ."""
context = ssl.create_default_context()
context = get_default_context()
context.set_ciphers(_LG_SSL_CIPHERS)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will mutate the default ssl context

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The simplest fix would be to create the context and store it in a constant so it doesn't happen at run time, and instead at import time which will run in the executor

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something like

diff --git a/custom_components/smartthinq_sensors/wideq/core_async.py b/custom_components/smartthinq_sensors/wideq/core_async.py
index 9335180..ec908ed 100644
--- a/custom_components/smartthinq_sensors/wideq/core_async.py
+++ b/custom_components/smartthinq_sensors/wideq/core_async.py
@@ -143,13 +143,18 @@ def _oauth_info_from_result(result_info: dict) -> dict:
 
     return result
 
+def _create_lg_ssl_context() -> ssl.SSLContext:
+    """Create a SSL context for LG ThinQ."""
+    context = ssl.create_default_context()
+    context.set_ciphers(_LG_SSL_CIPHERS)
+    return context
+
+_SSL_CONTEXT = _create_lg_ssl_context()
 
 def lg_client_session() -> aiohttp.ClientSession:
     """Create an aiohttp client session to use with LG ThinQ."""
-    context = ssl.create_default_context()
-    context.set_ciphers(_LG_SSL_CIPHERS)
     connector = aiohttp.TCPConnector(
-        enable_cleanup_closed=ENABLE_CLEANUP_CLOSED, ssl_context=context
+        enable_cleanup_closed=ENABLE_CLEANUP_CLOSED, ssl_context=_SSL_CONTEXT
     )
     return aiohttp.ClientSession(connector=connector)
 

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!!!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adjusted as suggested, yes this works without any warnings.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @iprak 👍

I'm far from home for work this month, I will work to merge this PR and prepare next release probably by the end of next week.

@iprak iprak force-pushed the fix-load_default_certs-warning branch from 070ebf6 to 2236bb0 Compare September 25, 2024 09:49
@ollo69 ollo69 merged commit 93599e2 into ollo69:master Oct 4, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warning load_default_certs in HA 2024.9
5 participants